Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

perf: machine string #1994

Merged
merged 3 commits into from
May 3, 2024
Merged

perf: machine string #1994

merged 3 commits into from
May 3, 2024

Conversation

petar-dambovaliev
Copy link
Contributor

@petar-dambovaliev petar-dambovaliev commented Apr 29, 2024

Make the String method on the Machine struct faster by preallocating a string builder.

issue

@github-actions github-actions bot added the 📦 🤖 gnovm Issues or PRs gnovm related label Apr 29, 2024
Copy link

codecov bot commented Apr 29, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 46.82%. Comparing base (f660be5) to head (20ea949).
Report is 13 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1994      +/-   ##
==========================================
+ Coverage   46.63%   46.82%   +0.18%     
==========================================
  Files         492      492              
  Lines       69624    69767     +143     
==========================================
+ Hits        32472    32670     +198     
+ Misses      34445    34388      -57     
- Partials     2707     2709       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@petar-dambovaliev petar-dambovaliev requested review from thehowl and removed request for thehowl April 29, 2024 13:03
@moul
Copy link
Member

moul commented Apr 29, 2024

do you have some benchmarks to share?

@petar-dambovaliev
Copy link
Contributor Author

do you have some benchmarks to share?

Benchmark pushed.

current branch

BenchmarkStringLargeData-10    	     212	   4518087 ns/op

master branch

BenchmarkStringLargeData-10    	     172	   6875883 ns/op

@petar-dambovaliev petar-dambovaliev marked this pull request as ready for review April 30, 2024 11:47
@petar-dambovaliev petar-dambovaliev requested review from zivkovicmilos, ajnavarro and thehowl and removed request for thehowl April 30, 2024 11:51
@deelawn
Copy link
Contributor

deelawn commented Apr 30, 2024

How long does it take to print the machine string if it exceeds memory allocation due to an infinitely recursing function?

@petar-dambovaliev
Copy link
Contributor Author

petar-dambovaliev commented Apr 30, 2024

How long does it take to print the machine string if it exceeds memory allocation due to an infinitely recursing function?

That issue is being fixed in another PR

gnovm/pkg/gnolang/gno_test.go Outdated Show resolved Hide resolved
gnovm/pkg/gnolang/machine.go Outdated Show resolved Hide resolved
@petar-dambovaliev petar-dambovaliev requested a review from deelawn as a code owner May 3, 2024 15:44
@petar-dambovaliev petar-dambovaliev merged commit 0fc011a into master May 3, 2024
199 checks passed
@petar-dambovaliev petar-dambovaliev deleted the machine-string branch May 3, 2024 16:01
Op: %v
Values: (len: %d)
%s
Exprs:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where does the new code print these headers? I can see one for Realm for not for the others

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📦 🤖 gnovm Issues or PRs gnovm related
Projects
Status: Done
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants